-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/hck 3426 re couchbase 7 instance re #5
Feature/hck 3426 re couchbase 7 instance re #5
Conversation
…get-document-kinds
buildConstants.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file about? buildConstants is it for esbuild?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll post a general remark here to highlight:
@Serhii Filonenko
There is something i’d like to raise
Organizing files is good that’s a fact but folders with a name like helpers and files inside named like Helper.js doesn’t really bring anything. It is just generic meaningless naming.
There are a few questions you could ask yourself that can help structure the code properly with meaningfull and better names:
- do the whole logic belongs only to reverse engineering, forward engineering or is it something of higher abstraction level? Here we are really mixing connection, ddl/storage structure (e.g objects and hierarchy), discovery with queries, and utility function like error handling.
- are the functions inside the files really focused on a single purpose (for instance if I look at clusterHelper.js all functions seem either related to buckets and some others are to discover top level structure of the Couchbase storage. So in terms of organization that already looks fishy (i would have a bucket.js and extract unrelated functions in other smaller modules for example a couchbaseErrorHandler.js or something in that vein, didn't spend much time reflecting on it)
- what are the real semantic concepts (like here many things are mixed up), i see functions to extract and discover database/storage structure and hierarchy e.g. Data Structure Definition (e.g DDL discovery). From my pov DDL/data structure should be top level and is not owned by RE nor FE, they both use DDL one way or another but they don’t own the logic.
- Ownership is important.
Don’t hesitate to have targetted folders with small focused modules with a super well focus and defined scope (it’s fine to have a module of a few lines because it helps tree shaking during bundling eventually and improve module isolation (purity?) and avoid unwanted side effects (+ easier unit testing)
…get-document-kinds
@bigorn0 Hi Ugo! Couldn't agree with you more, that we need to strive for a well-organized codebase as much as it makes sense. However, I don't think that "helpers" folder is completely meaningless, because devs can quickly detect these folders as a place for commonly used functions. Definitely, there is a room for decoupling the mentioned |
Looking at the content of helpers folder it is quite obvious the core logic is about retrieving/discovering database objects and hierarchy that we try to model so definitly, helpers has nothing to do with it. I could name it About the fact that dev can quickly identify commonly used functions then again, i don't get it because most of the functions inside that helpers folder are specific to Couchbase. Most of the functions inside are following an interface and implement what's expected by the application (REWorker scenario) for a given database structure discovery workflow then it is best to organize and surface that fact. I think there are enough abstractions and technics in Javascript to achieve that goal even without Typescript. https://app.gitbook.com/o/HBtg1gLTy0nw4NaX0MaV/s/k3cNzEeXGnIxGNnOQdDa/architecture/architectural-guidelines Already promotes the small, focused, well scoped module vs the GOD object approach that helpers, services, tools and other similarly named modules eventually become. |
…inds' of github.com:hackolade/CouchbaseV7Plus into Feature/HCK-3426-re-couchbase-7-instance-get-document-kinds
@bigorn0 Thank you for your feedback Ugo, the argumentation does make sense for me as well. We surely plan to continue refactoring that part of the implementation in next iterations. Again, the main intention is to deliver the working functionality that can be tested and verified, not delaying the release with an infinite rounds of reengineering it 👍🏻 |
Good, then I'm eager to see iteration 2 :) |
@bigorn0 Could you please check what's happening with the actions config? Seems like something misconfigured:
|
This reverts commit a4b6668.
Implemented:
_.default._default
collection